Conversation
ai-assisted=yes [TNZ-94638] [TNZ-94639]
|
Caution Review failedFailed to post review comments WalkthroughThis pull request enhances TLS certificate handling across the deployment system. Changes include: replacing insecure TLS client creation with CA-based certificate pool initialization in deployment deletion and preparation flows; adding a new SecureTLSClientMatcher() test helper to enforce TLS verification in mock expectations; introducing a CACertPool() method to the Certificate type for PEM-based certificate pool construction; and updating test suites to use the new matcher for stricter TLS validation. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors installation-manifest CA handling by centralizing CA parsing into a reusable helper and switching blobstore HTTP clients from InsecureSkipVerify to verified TLS using either system roots or a manifest-provided CA bundle.
Changes:
- Add
Certificate.CACertPool()to parse the installation manifest’s PEM CA into an*x509.CertPool(or returnnilto use system roots). - Update deployment prepare/delete flows to create blobstore clients with
CreateDefaultClient(certPool)instead ofCreateDefaultClientInsecureSkipVerify(). - Add/adjust tests: new unit tests for
CACertPool, plus a gomock matcher and test updates to assert blobstore clients are configured with TLS verification enabled.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| installation/manifest/manifest.go | Introduces CACertPool() helper to parse PEM CA into an *x509.CertPool. |
| installation/manifest/certificate_test.go | Adds unit coverage for CACertPool() (empty CA, valid PEM, invalid PEM). |
| cmd/suite_test.go | Adds a gomock matcher used by cmd tests to assert blobstore clients are configured for secure TLS verification. |
| cmd/deployment_preparer.go | Switches blobstore client creation to verified TLS, using CA cert pool from the installation manifest. |
| cmd/deployment_deleter.go | Switches blobstore client creation to verified TLS, using CA cert pool derived from the provided CA string. |
| cmd/deployment_deleter_test.go | Updates blobstore factory expectations to require a secure TLS client. |
| cmd/create_env_test.go | Updates blobstore factory expectations to require a secure TLS client and sets installationManifest.Cert.CA. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
No description provided.